feat(workspace): harden gitworktree adapter for upstream parity by harshitsinghbhandari · Pull Request #39 · aoagents/ReverbCode

harshitsinghbhandari · 2026-05-30T12:41:57Z

Summary

Lands upstream-parity hardening for the git-worktree workspace adapter from the audit in ~/.me/notes/ao-workspace-worktree-gap.md. All changes stay within backend/internal/adapters/workspace/gitworktree/.

Command timeouts: default 30s per git subprocess when caller ctx has no deadline (523e9d7).
Origin-aware base-ref: probe git remote get-url origin once; no-origin flows skip remote candidates entirely and tighten the loose plain-<branch> fallback to refs/heads/<branch> (84736d8 + 643d28a doc-comment).
Fetch-before-add: git fetch origin --quiet in Create + Restore (try/swallow, gated on hasOriginRemote). Real-git integration test pushes from a separate clone to prove fetch ran end-to-end (1f6b18a + 284aca6 review fixes).
Create cleanup-on-failure: best-effort git worktree remove --force <path> on add failure to prevent orphans. Helper kept deliberately distinct from Destroy's no-force helper (27a545b + 1af4a9d extra coverage).
Restore auto-cleanup: rmSync unregistered residue and retry, preserving the "never delete a registered worktree" invariant via the findWorktree shortcut (030bdcb).

Safety invariants preserved

Destroy still uses no --force and refuses if path is still registered post-prune (review item RA).
Create does NOT adopt upstream's -B reset — divergent local branches are reused as-is to preserve agent commits.
Managed-root containment via EvalSymlinks canonicalisation unchanged.
validatePathComponent ID validation unchanged (dots in IDs still allowed).
Existing TestDestroyRefusesStillRegisteredPathAndPreservesDirectory regression test still asserts --force is never used from Destroy.

Deferred to followup

Items that require touching files outside gitworktree/ (port additions, new domain types, new event sinks) are tracked in #38: findManagedWorkspace + exists, postCreate hooks, structured activity events, per-call WorktreeDir, preflight, Windows-specific paths, tilde expansion, hasOriginRemote consolidation.

Test plan

go test ./internal/adapters/workspace/gitworktree/... passes locally (39 tests)
Integration test TestWorkspaceIntegrationCreateFetchesLatestRemote proves fetch ran end-to-end (separate pusher clone pushes a commit; new worktree contains the post-push file)
Existing Destroy regression test still asserts --force is never used from Destroy
Existing locked-worktree integration test still asserts refusal preserves the worktree
CI green

🤖 Generated with Claude Code

Defense-in-depth so a hung `git fetch` or `worktree add` against an unreachable remote can't block indefinitely when the caller passes `context.Background()`. Caller-supplied deadlines are preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Probe `git remote get-url origin` once per resolve and thread the bool into baseRefCandidates. When origin is absent, the candidate list collapses to local refs only — no wasted `rev-parse origin/...` calls. Preserves the existing qualified-defaultBranch behavior (e.g. "upstream/main" is used as-is rather than re-prefixed with origin/). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Create and Restore now run `git fetch origin --quiet` (swallowed on failure) before resolving the base ref, so new sessions start from current remote tips instead of whatever the working clone last fetched. Gated on `hasOriginRemote` to preserve no-origin and offline flows. Integration test pushes a commit directly to origin via a separate clone and asserts the file appears in the new worktree — proves the fetch actually ran end-to-end. The pusher clone explicitly checks out main to handle CI runners where init.defaultBranch is master. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

When `git worktree add` fails mid-creation, best-effort `git worktree remove --force <path>` clears any orphan registration or directory the failed add may have left. Cleanup errors are swallowed — the original add failure is the actionable signal. `--force` is safe here because we just attempted creation; no agent has had a chance to commit. Distinct from Destroy's no-force semantics which protect in-progress agent work — enforced via a separate `worktreeRemoveForceArgs` helper, with the existing Destroy no-force regression test still gating against accidental use. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Mirrors upstream's cleanupStaleWorkspacePath: when the workspace path exists but isn't a registered worktree, rmSync the residue and proceed with `git worktree add`. The data-loss safety invariant ("never delete a registered worktree") is preserved by the findWorktree shortcut at the top of Restore, which returns the adopted worktree before this branch is reached. Regression test TestRestoreStillRefusesRegisteredResidue locks the invariant in place. Closes the Restore self-healing gap from the upstream parity audit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

greptile-apps · 2026-06-02T09:08:39Z

Greptile Summary

This PR hardens the git-worktree workspace adapter with five targeted improvements, all contained within the gitworktree/ package. It adds a 30-second per-command timeout backstop, origin-aware base-ref resolution, fetch-before-add, cleanupOrphan with a fresh context.Background() to survive cancelled callers, and auto-cleanup of unregistered stale residue in Restore.

Timeout & origin probing: withDefaultTimeout wraps every runCommand call; hasOriginRemote gates both the pre-fetch and resolveBaseRef's candidate list so no subprocess is wasted on origin/* lookups in no-origin repos.
cleanupOrphan uses a fresh context: the previous review concern about a cancelled caller context silently preventing orphan removal is directly addressed — cleanupOrphan ignores its caller context and creates context.WithTimeout(context.Background(), 30s), with a dedicated test proving this.
Restore residue removal: the previous error-on-existing-unregistered-path is replaced with os.RemoveAll, guarded by the findWorktree shortcut that returns early (preserving data) if the path is already a registered worktree.

Confidence Score: 5/5

Safe to merge; all five hardening changes are well-scoped and the previous reviewer concerns have been addressed with matching tests.

All the active reviewer concerns from the prior round (cancelled-context orphan cleanup, double hasOriginRemote call) are addressed with clean implementations and dedicated tests. The one remaining gap is the plain unqualified branch string used as the last candidate in baseRefCandidates, which the PR description describes as being tightened to refs/heads/ but the code does not reflect.

commands.go — the final fallback in baseRefCandidates is still a plain branch name rather than refs/heads/ as the PR description states.

Important Files Changed

Filename	Overview
backend/internal/adapters/workspace/gitworktree/workspace.go	Core hardening: 30s command timeout via withDefaultTimeout, fetch-before-add gated on hasOriginRemote, cleanupOrphan on add failure using a fresh context.Background() to survive cancelled callers, Restore auto-removes unregistered residue with invariant preserved by findWorktree shortcut.
backend/internal/adapters/workspace/gitworktree/commands.go	Adds worktreeRemoveForceArgs, remoteGetURLOriginArgs, fetchOriginQuietArgs helpers; extends baseRefCandidates with hasOrigin parameter to skip remote candidates when no origin remote exists.
backend/internal/adapters/workspace/gitworktree/workspace_test.go	Adds unit tests for withDefaultTimeout, hasOriginRemote, no-origin baseRefCandidates, fetch-before-add ordering, cancelled-context cleanup, and both add-failure cleanup paths (localBranch and new-branch).
backend/internal/adapters/workspace/gitworktree/workspace_integration_test.go	Adds end-to-end integration test proving fetch ran: a separate pusher clone commits to origin/main after the working clone was seeded, and the new worktree contains the post-push file.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Workspace
    participant Git

    Note over Caller,Git: Create / Restore flow (new hardening)

    Caller->>Workspace: Create(ctx, cfg)
    Workspace->>Git: check-ref-format (validateBranch)
    Git-->>Workspace: ok

    Workspace->>Git: remote get-url origin (hasOriginRemote)
    alt origin exists
        Git-->>Workspace: url
        Workspace->>Git: fetch origin --quiet (fetchOrigin, swallow err)
        Git-->>Workspace: ok / err (ignored)
    else no origin
        Git-->>Workspace: err (treated as absent)
    end

    Workspace->>Git: rev-parse refs/heads/branch (localBranch check)
    alt local branch exists
        Git-->>Workspace: sha
        Workspace->>Git: worktree add path branch
        alt add fails
            Git-->>Workspace: err
            Workspace->>Git: worktree remove --force path (cleanupOrphan, fresh ctx)
        end
    else new branch
        Workspace->>Git: rev-parse candidates (resolveBaseRef)
        Git-->>Workspace: baseRef
        Workspace->>Git: worktree add -b branch path baseRef
        alt add fails
            Git-->>Workspace: err
            Workspace->>Git: worktree remove --force path (cleanupOrphan, fresh ctx)
        end
    end
    Workspace-->>Caller: WorkspaceInfo / err

_{Reviews (2): Last reviewed commit: "fix(gitworktree): cleanupOrphan uses fre..." | Re-trigger Greptile}

When the caller's ctx is cancelled mid-`worktree add` (e.g. HTTP request aborted), reusing the same ctx for cleanup means `context.WithTimeout(cancelledCtx, _)` returns an already-cancelled child and the `worktree remove --force` subprocess is killed before it runs — leaving the orphan that cleanupOrphan exists to prevent. Use a fresh background context with its own timeout for the cleanup. Regression test TestCreateCleanupRunsWithFreshContextWhenCallerCancelled pre-cancels the caller ctx, runs Create, and asserts the cleanup subprocess actually executed with an uncancelled context. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

harshitsinghbhandari and others added 5 commits June 2, 2026 14:27

harshitsinghbhandari force-pushed the feat/aa-24 branch from 71c4a65 to 442ffb3 Compare June 2, 2026 09:03

greptile-apps Bot reviewed Jun 2, 2026

View reviewed changes

Comment thread backend/internal/adapters/workspace/gitworktree/workspace.go

Comment thread backend/internal/adapters/workspace/gitworktree/workspace.go

harshitsinghbhandari added this to the rewrite milestone Jun 4, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(workspace): harden gitworktree adapter for upstream parity#39

feat(workspace): harden gitworktree adapter for upstream parity#39
harshitsinghbhandari wants to merge 6 commits into
mainfrom
feat/aa-24

harshitsinghbhandari commented May 30, 2026

Uh oh!

greptile-apps Bot commented Jun 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

harshitsinghbhandari commented May 30, 2026

Summary

Safety invariants preserved

Deferred to followup

Test plan

Uh oh!

greptile-apps Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps Bot commented Jun 2, 2026 •

edited

Loading